Skip to content

chore(lint): fix pre-existing golangci-lint issues#87

Merged
cnjack merged 1 commit into
mainfrom
chore/lint-cleanup
Jun 20, 2026
Merged

chore(lint): fix pre-existing golangci-lint issues#87
cnjack merged 1 commit into
mainfrom
chore/lint-cleanup

Conversation

@cnjack

@cnjack cnjack commented Jun 20, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Owner

Summary

  • Cleans up the 5 trivial (non-deprecation) issues reported by golangci-lint run ./... that don't block CI (only-new-issues: true) but were flagged by the new quality gate.
  • No behavior change — these are mechanical lint fixes only.

Related Issues / Tickets

  • N/A — follow-up to the new CI quality gate in .github/workflows/ci.yml.

Type of Change

  • Refactoring / code cleanup

Changes Made

  1. unparam (goal.go) — dropped the unused *Goal return from (*GoalStore).setStatus; both callers (Complete/Block) only used the bool.
  2. goimports (goal_test.go) — moved the internal/session import into its own third-party group.
  3. ineffassign (ssh.go) — replaced remotePwd := "/root" with var remotePwd string; the initializer was always overwritten by the following if/else.
  4. gocritic / deprecatedComment (config.go) — moved the AutoApprove Deprecated: notice into its own paragraph.
  5. gocritic / ifElseChain (acp.go) — rewrote the team_send_message if-else chain as a tagless switch.

Testing

  • golangci-lint run ./... → only the 9 adk.AgentMiddleware SA1019 deprecation warnings remain (intentionally left for the separate dependency-migration task); 0 other issues.
  • go build ./... → OK
  • go test ./... → all packages pass

Additional Notes

Fixing the Deprecated: comment format (#4) made staticcheck correctly recognize AutoApprove as deprecated, surfacing two new SA1019 warnings on intentional fallback reads (interactive.go, tui.go). Those reads are documented behavior — the field is still honored as a fallback when DefaultMode is unset — so they're annotated with //nolint:staticcheck rather than removed. This keeps the remaining-warning count at exactly the 9 adk.AgentMiddleware items.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Refactor

    • Improved internal code structure and helper method implementations for better maintainability.
    • Enhanced conditional logic formatting for improved readability.

  • Tests

    • Updated test imports for consistency.

  • Chores

    • Added linter directives to preserve backward compatibility handling.

@cnjack @claude
chore(lint): fix pre-existing golangci-lint issues
5965d29 
Clean up the trivial (non-deprecation) issues reported by golangci-lint
that don't block CI but were flagged by the new quality gate:

- unparam: drop the unused *Goal return from (*GoalStore).setStatus;
  both callers only used the bool.
- goimports: regroup the third-party import in goal_test.go.
- ineffassign: replace `remotePwd := "/root"` with `var remotePwd string`
  in ssh.go since the initializer was always overwritten.
- gocritic (deprecatedComment): move the AutoApprove `Deprecated:` notice
  into its own paragraph.
- gocritic (ifElseChain): rewrite the team_send_message if-else chain in
  acp.go as a switch.

Fixing the Deprecated: comment format made staticcheck correctly
recognize AutoApprove as deprecated, surfacing two SA1019 warnings on
intentional fallback reads. Those reads are documented behavior (the
field is still honored when DefaultMode is unset), so they are annotated
with //nolint:staticcheck rather than removed.

The 9 remaining adk.AgentMiddleware SA1019 warnings are intentionally
left for the separate dependency-migration task.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 186822c4-d7a8-4a46-bc90-8246d1c93269

📥 Commits

Reviewing files that changed from the base of the PR and between 476ff82 and 5965d29.

📒 Files selected for processing (7)
  • internal/command/interactive.go
  • internal/command/ssh.go
  • internal/config/config.go
  • internal/handler/acp.go
  • internal/tools/goal.go
  • internal/tools/goal_test.go
  • internal/tui/tui.go
📝 Walkthrough

Walkthrough

A collection of small independent cleanups: GoalStore.setStatus is refactored to return only a bool instead of a snapshot plus bool; HandleSSHConnect removes the hardcoded "/root" default for remotePwd; presentationForTool switches from an if/else chain to a switch statement; and //nolint:staticcheck comments are added to two usages of the deprecated AutoApprove field.

Changes

Miscellaneous Cleanups

Layer / File(s) Summary
GoalStore.setStatus return type simplification
internal/tools/goal.go, internal/tools/goal_test.go		 setStatus now returns only a bool instead of a cloned goal snapshot plus bool; Complete() and Block() are simplified to directly return that bool; test file import order is adjusted.
SSH default removal and acp.go switch refactor
internal/command/ssh.go, internal/handler/acp.go		 remotePwd in HandleSSHConnect is declared uninitialized instead of defaulting to "/root"; team_send_message title selection is rewritten as a switch on the to value.
AutoApprove deprecation suppressions
internal/command/interactive.go, internal/tui/tui.go, internal/config/config.go		 //nolint:staticcheck comments are added to the two cfg.AutoApprove fallback branches; a blank comment line is inserted in the Config struct near the AutoApprove field.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~4 minutes

Poem

🐇 A nolint here, a switch right there,
No more hardcoded /root to bear,
setStatus returns just a bool now,
Cleaner code — and how!
The warren's tidy, hip-hip-hooray! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'chore(lint): fix pre-existing golangci-lint issues' directly and accurately summarizes the main change—addressing golangci-lint issues across multiple files with mechanical lint fixes and no behavioral impact.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/lint-cleanup

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cnjack cnjack merged commit a3fbf9e into main Jun 20, 2026
3 checks passed
@cnjack cnjack deleted the chore/lint-cleanup branch June 20, 2026 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@cnjack